Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reCAPTCHA: Error pages #2585

Open
wants to merge 15 commits into
base: refactor/recaptcha-copy
Choose a base branch
from

Conversation

machikoyasuda
Copy link
Member

@machikoyasuda machikoyasuda commented Dec 11, 2024

closes #2545

What this PR does

  • Realign all the error pages
  • Refactors error pages so they all inherit from 1 base file, now called error-base.html (previously was error.html). Now both Admin and Benefits have an error-base. The base naming is used like this now:
    image, to let the developer know that this is a template that inherits from base.html, and other templates then inherit it.
  • error-base.html now makes it so that there is one place to make these changes, making it easier to review also. When making a new error page that is based on this template/design, the developer should not have to write any padding/margin/centering/alignment classes or code. They should only have to write copy (headline-text), create a new or use an existing includes for an icon, add any <p> of text, and use an existing button, or create a new one (the button might have existing CSS classes, like login). But there shouldn't be a need for any new rows or justify-content-centers or col-lg-6. That is the responsibility of the error-base template.

How error-base works:

{% extends "core/base.html" %}  /* inherits from base, so any blocks in base can be used - even if not specified here */
{% load i18n %}

{% block page-title %}
  {% block title %}
  {% endblock title %}
{% endblock page-title %}

/* inherits from base, so you could put a block for nav-buttons here to have a nice area to put the log out button */

{% block inner-content %}   /* inherits from base, `inner-content` has a `<div class="row justify-content-center`"> outside of it, so we can omit it. you have 2 options here: override inner-content entirely, OR, onlly override the children: icon, headline-text, paragraphs. all of our error pages now do this, but in the future if there is some weird new one-off error design where these elements are in a different order, we can override inner-content directly*/
  <div class="col-lg-6">
    <div class="mt-5 pt-4 text-center">
      {% block icon %} /* put the icon includes in here directly, no wrapper div needed */
      {% endblock icon %} /* Default: sadbus icon. If sadbus, then don't need to include this block at all */
    </div>
    <h1 class="h2 py-4">
      {% block headline-text %} /* put the text in here directly, no wrapper div needed */
      {% endblock headline-text %}
    </h1>
    {% block paragraphs %} /* put paragraph elements and other includes (like agency-links) in here directly, no wrapper div needed */
    {% endblock paragraphs %}
  </div>
{% endblock inner-content %}

{% block call-to-action-button %} /* the logic here is, "show the button to home page by default if a template doesn't specify one. or override the button by putting in your own block. */
  {% include "core/includes/button--origin.html" %}
{% endblock call-to-action-button %}

Error pages

  1. 500
  2. 404
  3. 400
  4. 200 user error
  5. OAuth system error
  6. Enrollment system error
  7. Enrollment retry
  8. Re-enrollment CalFresh
  9. Eligibility unverified

Side note outside of the scope of this PR/Milestone

  • The transit agency name is supposed to be bold on the agency-links, but it's not. Not part of this PR.
  • I noticed an inconsistency in button text casing. There's Start Here and Return home and Try again. Not part of this PR.
  • Some files are set to 4 space tabs, while most are 2: reenrollment-error--calfresh and start--medicare at least. Not sure why. Not part of this PR.
  • Testing these pages is kinda cumbersome. Perhaps we could have some sort of helper file / method that makes it easier to access all of them.

@machikoyasuda machikoyasuda requested a review from a team as a code owner December 11, 2024 17:24
@github-actions github-actions bot added tests Related to automated testing (unit, UI, integration, etc.) front-end HTML/CSS/JavaScript and Django templates and removed tests Related to automated testing (unit, UI, integration, etc.) labels Dec 11, 2024
Copy link

github-actions bot commented Dec 11, 2024

Coverage report

This PR does not seem to contain any modification to coverable code.

@machikoyasuda machikoyasuda force-pushed the feat/2545-recaptcha-errors branch from 37c0630 to 817bae4 Compare December 11, 2024 17:31
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't change or touch this file at all.... the linter did this!

benefits/templates/200-user-error.html Outdated Show resolved Hide resolved
benefits/templates/error-base.html Show resolved Hide resolved
@machikoyasuda machikoyasuda force-pushed the feat/2545-recaptcha-errors branch from e24f966 to b47401d Compare December 12, 2024 01:23
@machikoyasuda
Copy link
Member Author

I noticed that the transit agency name bolding on agency links is a regression on main. See more here: #2590 So I made a bug ticket and I can work on it after reCAPTCHA is done, in this sprint or immediately in slush.

@machikoyasuda machikoyasuda force-pushed the feat/2545-recaptcha-errors branch from d38b680 to 812446c Compare December 12, 2024 19:16
Copy link
Member

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice clean ups 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
front-end HTML/CSS/JavaScript and Django templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants